Add the source@v1 protocol layer - #112
Conversation
a49c06d to
8b8a514
Compare
There was a problem hiding this comment.
🟡 Changes recommended
A new test comment violates the repository’s documentation convention by recording transient external implementation history.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds the internal source@v1 protocol foundation for issue #95.
Changes:
- Adds source role identifiers, messages, serialization, and command parsing.
- Centralizes big-endian 64-bit timestamp helpers.
- Adds protocol coverage for source messages and timestamp encoding.
File summaries
| File | Description |
|---|---|
src/protocol_messages.h |
Defines source protocol types and endian helpers. |
src/protocol.cpp |
Parses and serializes source messages. |
src/player_role.cpp |
Uses shared timestamp decoding. |
src/artwork_role.cpp |
Uses shared timestamp decoding. |
src/visualizer_role.cpp |
Uses shared timestamp decoding. |
tests/test_protocol.cpp |
Tests source protocol and endian behavior. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
There was a problem hiding this comment.
🔵 Needs a closer look
The unresolved timestamp-size and shift-width duplication issues must be addressed before approval.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
src/protocol_messages.h:94
- Derive this shift from
BINARY_TIMESTAMP_SIZEinstead of duplicating its 8-byte invariant as56. The loop bound and shift width can otherwise drift independently, contrary to the derived-constants rule indocs/conventions.md:69-71.
src/visualizer_role.cpp:535 - Please complete the timestamp-size dedup here. This decoder now consumes
BINARY_TIMESTAMP_SIZE, but the visualizer's bounds checks and payload offsets still use an independentTIMESTAMP_SIZE = 8at lines 36, 224, 529, and 584-585. If the shared wire size changes, those paths can validate or offset a different width than this helper reads. Replace the local constant throughout withBINARY_TIMESTAMP_SIZE(or derive it from that constant), as required bydocs/conventions.md:69-71.
src/protocol_messages.h:641
- Replace the “has no role class ... yet” rationale with a durable statement of current visibility. This is phase language that will become stale as the remaining source stack lands;
docs/conventions.md:123-126requires comments to avoid phase/porting language.
// Unlike the sibling sections above, these types are not mirrored from a public header: the
// source role has no role class or include/sendspin/ header yet, so its protocol types are
// internal-only.
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The be64 encoder's shift and the visualizer's entry-layout constant both restated BINARY_TIMESTAMP_SIZE's 8-byte invariant as independent literals; both now derive from it. Also trims the source-section comment to the durable section marker.
There was a problem hiding this comment.
🟡 Changes recommended
Message envelopes and codec-header validation must be corrected to conform to the protocol.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 4
- Review effort level: Balanced
Emit the required empty payload object on client-stream/end (Sendspin spec, Message Format: every message carries a payload object), derive the 4-slot binary audio type ids from their role values so they cannot desynchronize, state the codec_header per-codec invariant at the message struct, and drop a cross-project history reference from a test comment.
There was a problem hiding this comment.
🔵 Needs a closer look
The serialization test must validate the protocol-required JSON boolean type.
Review details
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
tests/test_protocol.cpp:812
- This assertion converts the value to
bool, so it would still pass if the serializer emitted numeric1instead of the protocol-required JSON boolean. Assertis<bool>()first, as the existing mute serialization test does, so this test protects the wire type as well as the truth value.
src/protocol_messages.h:707
- Adding source state makes the struct’s
@briefinaccurate:ClientStateMessageno longer reports only playback state. Please describe it as client/role state so the internal protocol documentation matches the fields it contains.
std::optional<ClientSourceStateObject> source{};
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The hello test now pins line_sense as a JSON boolean (the mute tests' pattern) rather than a coercible truthy value, and ClientStateMessage's brief covers the role state it carries alongside availability.
|
Both items from the latest review round fixed in 506a293: the hello test now asserts |
Part 1/6 of the source@v1 stack (tracker: #95). Each part is based on the previous one.
What it adds: the source@v1 protocol layer — the source role/binary ids (chunk type 12), the
client-stream/start/client-stream/endserializers,server/commandsource parsing, and shared big-endian 64-bit read/write helpers (the player/artwork/visualizer readers now use them).How it's used: nothing calls the new pieces yet; part 3's source role uses them to announce and frame its outbound audio. The be64 dedup is a pure refactor covered by the existing suites.